Skip to content

Feature/spark wallet#165

Open
satoshisound wants to merge 3 commits intomasterfrom
feature/spark-wallet
Open

Feature/spark wallet#165
satoshisound wants to merge 3 commits intomasterfrom
feature/spark-wallet

Conversation

@satoshisound
Copy link
Copy Markdown
Member

@satoshisound satoshisound commented Apr 15, 2026

Note

Medium Risk
Touches core wallet flows (payments, receive/send UI, settings persistence) and adds new IPC-based Spark backend integration; failures could impact wallet UX and routing behavior but changes are largely additive and gated behind optional Spark availability.

Overview
Adds initial Spark wallet support to the GUI alongside Liquid, including new Menu::Spark navigation and state panels (Overview/Send/Receive/Transactions/Settings) plus an app-level Spark event subscription that refreshes Spark UI on bridge events.

Introduces a new coincube-spark-protocol crate and a gui-side SparkClient that talks to an external coincube-spark-bridge subprocess over JSON-lines stdin/stdout (workspace explicitly excludes the bridge to avoid dependency conflicts).

Refactors Liquid-facing UI code to consume wallet-agnostic domain types via a new app::wallets layer (LiquidBackend, WalletRegistry) and updates cube settings/caching to store a unified Breez signer fingerprint plus a per-cube default_lightning_backend setting (with a new Settings → Lightning section) used to route incoming Lightning Address invoices.

Reviewed by Cursor Bugbot for commit a6b27d0. Bugbot is set up for automated code reviews on this repo. Configure here.

Summary by CodeRabbit

  • New Features

    • Spark wallet added (backend + subprocess bridge) with Overview, Send, Receive, Transactions, Settings
    • Wallet routing and Default Lightning backend selection (Spark or Liquid)
    • Stable Balance toggle and on-chain deposit claiming for Spark
    • Export Spark transaction CSV and LNURL/Lightning-address routing improvements
  • Style

    • Updated orange brand palette and new orange-outline button style and Spark badge asset
  • Documentation

    • Added Spark wallet and multi-wallet integration guides
  • Chores

    • Expanded .env.example with unified BREEZ_API_KEY and clearer geolocation notes

@satoshisound satoshisound requested a review from sokorototo April 15, 2026 15:55
@coderabbitai
Copy link
Copy Markdown

coderabbitai bot commented Apr 15, 2026

📝 Walkthrough

Walkthrough

Adds Spark wallet support via a new JSON‑RPC protocol crate and a subprocess bridge for the Breez Spark SDK, introduces GUI-side Spark client/backend, domain wallet abstractions (Liquid/Spark), Spark state/views (Overview/Send/Receive/Transactions/Settings), and related wiring, plus .env and workspace updates.

Changes

Cohort / File(s) Summary
Workspace & Env
Cargo.toml, .env.example
Workspace updated to add coincube-spark-protocol and exclude coincube-spark-bridge; .env.example comments clarified and a BREEZ_API_KEY= entry added/relocated (geolocation docs expanded).
Protocol & Bridge
coincube-spark-protocol/..., coincube-spark-bridge/Cargo.toml, coincube-spark-bridge/src/{main.rs,sdk_adapter.rs,server.rs}
New protocol crate defining Frame/Request/Response/Event types and a new bridge binary implementing JSON‑RPC over stdio, SDK adapter, request handlers, event forwarding, and a smoketest path.
GUI Spark Client & Config
coincube-gui/src/app/breez_spark/{client.rs,config.rs,assets.rs}, coincube-gui/Cargo.toml
Adds SparkClient managing bridge subprocess, concurrent RPCs, event subscriptions, SparkConfig loading via compile-time env, and a simple Spark asset enum; GUI Cargo.toml gains spark-protocol path dependency and expanded tokio features.
Wallet Abstraction & Backends
coincube-gui/src/app/wallets/{mod.rs,types.rs,liquid.rs,spark.rs,registry.rs}
Introduces domain wallet types (DomainPayment, DomainPaymentDetails, DomainPaymentStatus, WalletKind) and backend adapters LiquidBackend/SparkBackend plus WalletRegistry for routing Lightning requests.
Spark State & Views
coincube-gui/src/app/state/spark/..., coincube-gui/src/app/view/spark/...
Adds Spark panel state machines (overview/send/receive/transactions/settings) and corresponding view renderers, QR handling, deposit claim flows, stable-balance UI, and export support.
Liquid Reorg & Domain Migration
coincube-gui/src/app/breez_liquid/*, coincube-gui/src/app/state/liquid/*, coincube-gui/src/app/view/liquid/*
Renames/segregates Breez Liquid integration to breez_liquid, replaces SDK types with domain types across Liquid panels and views, and adapts code to use LiquidBackend.
Core App Integration & Routing
coincube-gui/src/app/{mod.rs,menu.rs,message.rs,cache.rs,settings/mod.rs,state/mod.rs,view/mod.rs}
Adds Spark menu/messages, app-level wallet registry and optional Spark backend wiring, cache/settings fields (spark_expanded, cube_id, default_lightning_backend), and routes/events to Spark panels.
Startup & UI plumbing
coincube-gui/src/{gui/tab.rs,loader.rs,installer/mod.rs,launcher.rs,installer/...}
Threads SparkBackend through loader/installer/launcher flows, attempts non-fatal Spark load alongside Liquid after PIN, and updates constructors/signatures to accept optional Spark backend.
Services & Export
coincube-gui/src/services/lnurl/*, coincube-gui/src/export.rs, coincube-gui/src/services/connect/login.rs
LNURL flow extended to optionally route invoice minting to Spark based on preferred backend + description/hash checks; InvoiceRequestEvent gains description; export pipeline updated to support Spark export and Liquid export via domain backends.
UI Theming & Helpers
coincube-ui/src/{color.rs,component/button.rs,theme/*,image.rs}
Adds orange_outline button style, new orange palette values, Spark badge SVG and network logo handling, and UI button helper for orange_outline.
Docs
docs/SPARK_WALLET.md, docs/WALLETS.md
New documentation describing Spark wallet architecture, bridge subprocess design, routing, and wallet abstraction guidance.

Sequence Diagram(s)

sequenceDiagram
    participant GUI as GUI App
    participant SparkClient as SparkClient
    participant Bridge as Bridge subprocess
    participant SDK as Breez Spark SDK

    GUI->>SparkClient: connect(config, mnemonic)
    SparkClient->>Bridge: spawn bridge process (stdin/stdout)
    SparkClient->>Bridge: send Init request (JSON-line)
    Bridge->>SDK: connect_mainnet(mnemonic, api_key, storage_dir)
    SDK-->>Bridge: Init OK
    Bridge-->>SparkClient: Init response
    SparkClient-->>GUI: Connected (event subscription)

    rect rgba(0, 100, 200, 0.5)
      Note over SparkClient,Bridge: ongoing background tasks
      SparkClient->>Bridge: writer task -> write Request frames
      Bridge->>SparkClient: reader task -> Event/Response frames
      Bridge->>SDK: SDK emits events (PaymentSucceeded, DepositsChanged)
      Bridge-->>SparkClient: Event frames forwarded
      SparkClient-->>GUI: broadcast events
    end
Loading
sequenceDiagram
    participant View as SparkSendView
    participant State as SparkSendState
    participant Backend as SparkBackend
    participant Bridge as Bridge subprocess
    participant SDK as Breez Spark SDK

    View->>State: PrepareRequested(destination, amount)
    State->>Backend: parse_input(destination)
    Backend->>Bridge: ParseInput request
    Bridge->>SDK: classify input
    SDK-->>Bridge: ParseInputOk
    Bridge-->>Backend: ParseInputOk
    alt LNURL / Lightning
        State->>Backend: prepare_lnurl_pay(...)
    else On-chain / Bolt11
        State->>Backend: prepare_send(...)
    end
    Backend->>Bridge: PrepareSend / PrepareLnurlPay
    Bridge->>SDK: SDK prepare
    SDK-->>Bridge: PrepareSendOk(handle, fee)
    Bridge-->>Backend: PrepareSendOk
    Backend-->>State: Prepared -> show preview
    View->>State: ConfirmRequested
    State->>Backend: send_payment(handle)
    Backend->>Bridge: SendPayment request
    Bridge->>SDK: execute send
    SDK-->>Bridge: SendPaymentOk
    Bridge-->>Backend: SendPaymentOk
    Backend-->>State: Sent -> View shows success
Loading

Estimated code review effort

🎯 5 (Critical) | ⏱️ ~135 minutes

Possibly related PRs

Suggested reviewers

  • sokorototo
  • QaidVoid

Poem

🐰 A tiny bridge hums, a Spark in the night,

I tunneled a protocol, framed it just right.
Liquid and Spark now dance, each with its part,
Events hop like carrots, safe-guarded by heart.
Hooray — a rabbit cheer for this bright new chart! 🥕✨

✨ Finishing Touches
📝 Generate docstrings
  • Create stacked PR
  • Commit on current branch
🧪 Generate unit tests (beta)
  • Create PR with unit tests
  • Commit unit tests in branch feature/spark-wallet
⚔️ Resolve merge conflicts
  • Resolve merge conflict in branch feature/spark-wallet

Comment thread coincube-gui/src/app/breez_spark/client.rs
Comment thread coincube-gui/src/app/state/spark/receive.rs
Comment thread coincube-gui/src/app/wallets/types.rs
Copy link
Copy Markdown

@coderabbitai coderabbitai bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actionable comments posted: 18

Caution

Some comments are outside the diff and can’t be posted inline due to platform limitations.

⚠️ Outside diff range comments (4)
coincube-gui/src/loader.rs (1)

858-865: ⚠️ Potential issue | 🟠 Major

Replace std::thread::sleep with async sleep to avoid blocking the executor thread.

sync() runs under Task::perform, which uses the tokio runtime. std::thread::sleep parks the executor worker for a full second on every poll with sleep=true, stalling unrelated async work like wallet tasks and UI updates. Use tokio::time::sleep(...).await instead, which is already available in your dependencies and used elsewhere in the codebase.

Suggested fix
 async fn sync(
     daemon: Arc<dyn Daemon + Sync + Send>,
     sleep: bool,
 ) -> Result<GetInfoResult, DaemonError> {
     if sleep {
-        std::thread::sleep(std::time::Duration::from_secs(1));
+        tokio::time::sleep(std::time::Duration::from_secs(1)).await;
     }
     daemon.get_info().await
 }
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.

In `@coincube-gui/src/loader.rs` around lines 858 - 865, The sync function is
blocking the tokio executor by calling std::thread::sleep; replace that call in
async fn sync(...) with an asynchronous sleep using
tokio::time::sleep(std::time::Duration::from_secs(1)).await so the task yields
instead of parking the worker thread; keep the existing daemon.get_info().await
return behavior and add the tokio::time::sleep call only when sleep == true.
coincube-gui/src/services/lnurl/stream.rs (1)

21-42: ⚠️ Potential issue | 🟠 Major

Include routing inputs in the subscription identity.

create_stream captures preferred and spark_backend, but this Hash implementation ignores both. If the user changes default_lightning_backend or the Spark bridge comes up/down while token and retries stay the same, the existing SSE task keeps minting invoices with the stale route until the subscription happens to restart for some other reason.

Suggested fix
 impl Hash for LnurlStreamData {
     fn hash<H: Hasher>(&self, state: &mut H) {
         self.token.hash(state);
         self.retries.hash(state);
+        matches!(self.preferred, WalletKind::Spark).hash(state);
+        self.spark_backend.is_some().hash(state);
     }
 }
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.

In `@coincube-gui/src/services/lnurl/stream.rs` around lines 21 - 42, The Hash
impl for LnurlStreamData currently only hashes token and retries, so changes to
routing inputs are ignored; update the Hash implementation for LnurlStreamData
to also incorporate the routing-related fields used by
create_stream/handle_invoice_request: include preferred (WalletKind) and the
spark_backend identity (e.g., hash spark_backend.is_some() or the Arc
pointer/inner identity) so that changes to default_lightning_backend or the
Spark bridge presence will change the stream's hash and force Iced to recreate
the SSE task.
coincube-gui/src/gui/tab.rs (1)

975-984: ⚠️ Potential issue | 🟠 Major

Remote-backend sessions still drop Spark on app creation.

BreezClientLoadedAfterPin now loads Spark, but create_app_with_remote_backend has no Spark parameter and hardcodes None into App::new. That means remote-backend users will never see Spark after login, even when the backend was loaded successfully during PIN verification.

Also applies to: 1114-1115

🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.

In `@coincube-gui/src/gui/tab.rs` around lines 975 - 984, The
create_app_with_remote_backend function currently hardcodes None for the Spark
argument when calling App::new, which prevents Spark loaded by
BreezClientLoadedAfterPin from being visible; change the signature of
create_app_with_remote_backend to accept the Spark (or Option<SparkType>)
parameter and forward that value into app::App::new instead of None, then update
all call sites (including where BreezClientLoadedAfterPin returns a loaded Breez
client/PIN flow) to pass the loaded Spark through; ensure the type matches the
App::new parameter and adjust related declarations where the function is
defined/used (also update the other occurrence around lines 1114-1115) so
remote-backend users receive the loaded Spark after login.
coincube-gui/src/app/state/liquid/send.rs (1)

790-833: ⚠️ Potential issue | 🟠 Major

Preserve USDt classification even when asset_info is missing.

load_balance already treats asset_id == usdt_id as the USDt check, but this block switches back to asset_info.as_ref().map(...) and uses that to set is_usdt_payment. If a USDt payment arrives without asset_info, the row falls back to BTC formatting/fiat conversion and loses the USDt description. Use asset_id to classify the asset, and only use amount_minor when it is available.

🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.

In `@coincube-gui/src/app/state/liquid/send.rs` around lines 790 - 833, The USDt
classification must use the asset_id check instead of relying on asset_info
presence: set is_usdt_payment by matching payment.details against
DomainPaymentDetails::LiquidAsset { asset_id, .. } and comparing asset_id ==
usdt_id; compute usdt_amount_minor separately as asset_info.as_ref().map(|i|
i.amount_minor) only when asset_info exists; keep amount =
Amount::from_sat(usdt_amount_minor.unwrap_or(payment.amount_sat)); keep
fiat_amount = None when is_usdt_payment else use fiat_converter.convert(amount);
and adjust the desc match to detect USDt by asset_id (or is_usdt_payment) but
avoid depending on asset_info for choosing the "USDt Transfer" fallback.
🧹 Nitpick comments (3)
docs/SPARK_WALLET.md (1)

13-25: Add a language specifier to the fenced code block.

The ASCII diagram fenced code block is missing a language specifier. Use an empty specifier or text to satisfy markdown linting.

📝 Suggested fix
-```
+```text
 ┌──────────────┐         stdin/stdout           ┌──────────────────────┐
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.

In `@docs/SPARK_WALLET.md` around lines 13 - 25, The fenced ASCII diagram (the
triple backticks surrounding the diagram in the docs/SPARK_WALLET.md file) is
missing a language specifier; update the opening fence from ``` to ```text (or
an empty specifier) so the block is marked as plain text and satisfies the
markdown linter—edit the code block that contains the ASCII diagram (the block
starting with the triple backticks above the diagram) to include the specifier.
coincube-spark-bridge/Cargo.toml (1)

22-23: Consider documenting the lint override reason.

The mismatched_lifetime_syntaxes lint is allowed, which may be necessary due to upstream crate code generation. A brief comment explaining why this override is needed would aid future maintenance.

📝 Suggested documentation
 [lints.rust]
+# breez-sdk-spark generates code with lifetime syntax that triggers this lint
 mismatched_lifetime_syntaxes = "allow"
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.

In `@coincube-spark-bridge/Cargo.toml` around lines 22 - 23, Add a brief comment
next to the Cargo.toml lints override explaining why
mismatched_lifetime_syntaxes is set to "allow" (e.g., due to upstream/generated
code or a specific dependency causing unavoidable lifetime syntax mismatches).
Locate the [lints.rust] section and the mismatched_lifetime_syntaxes entry and
insert a one-line rationale referencing the upstream crate or codegen that
necessitates this override so future maintainers understand the exception.
coincube-gui/src/app/view/liquid/send.rs (1)

1881-2041: Remove the parked legacy toggle before merge.

Keeping the full _old_send_asset_toggle implementation around makes this file harder to maintain and easy to accidentally resurrect once the new picker flow diverges.

🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.

In `@coincube-gui/src/app/view/liquid/send.rs` around lines 1881 - 2041, Delete
the parked legacy toggle by removing the entire _old_send_asset_toggle function
(including the leading NOTE comment and the #[allow(dead_code)] attribute) from
the file; ensure you also remove any leftover references to
_old_send_asset_toggle and its symbol (fn _old_send_asset_toggle, SendAsset
variants used only there) so the compiler/warnings stay clean, and run a quick
build to confirm no callers remain (e.g. search for _old_send_asset_toggle and
LiquidSendMessage::PresetAsset usages that were only for this old toggle).
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.

Inline comments:
In @.env.example:
- Line 21: The .env.example currently ends immediately after the
BREEZ_SPARK_API_KEY= line and triggers the EndingBlankLine lint; add a single
trailing newline at the end of the file so the file ends with a blank line
(ensure the final character after the BREEZ_SPARK_API_KEY= line is a newline).

In `@coincube-gui/src/app/breez_spark/client.rs`:
- Around line 483-497: In shutdown(), the code sets inner.closed (via
closed.swap(true, Ordering::SeqCst)) before calling request(Method::Shutdown),
which causes request() to immediately return BridgeUnavailable; change the logic
so the Shutdown RPC actually gets sent: either move the closed.swap(true, ...)
to after awaiting self.request(Method::Shutdown) inside shutdown(), or update
request() to special-case Method::Shutdown and allow the call even when
inner.closed is true; ensure the same fix is applied to the equivalent block
around lines 525-533 (the alternate shutdown path).
- Around line 690-747: On stdout close or read error inside spawn_reader_task,
drain the pending map and fail each outstanding oneshot by taking
pending.lock().await, removing all entries (e.g., via drain or iterating keys),
and sending an immediate error/response to each sender so callers don't wait the
full timeout; perform this cleanup in both the Ok(None) and Err(e) branches
before breaking. Also mark the client as closed (set the shared "closed" flag or
notify the client-closed signal used by the client code) after failing pending
requests so other tasks see the subprocess is dead. Ensure you update only
symbols shown here: spawn_reader_task, pending (PendingMap), and event_tx/Frame
handling, and send a failure through each oneshot::Sender previously stored in
pending.

In `@coincube-gui/src/app/breez_spark/config.rs`:
- Around line 1-8: The current api_key_from_env() bakes an empty string via
option_env!(...).unwrap_or("") which hides missing credentials; change it to
enforce compile-time or explicit failure: either replace option_env! with
env!(...) so missing BREEZ_SPARK_API_KEY / BREEZ_API_KEY triggers a compile
error (mirroring the Breez Liquid pattern), or keep option_env! but return a
Result and map a missing key to a SparkConfigError (e.g., return
Err(SparkConfigError::MissingApiKey)) so callers of
api_key_from_env()/SparkConfig::new() fail early instead of embedding an empty
API key; update api_key_from_env, SparkConfig::new, and any callers to handle
the Result accordingly.

In `@coincube-gui/src/app/settings/mod.rs`:
- Around line 178-183: The doc is wrong because serde(default) will use
WalletKind::default() (currently Spark); fix by providing an explicit
deserialization default that returns Liquid: add a function (e.g.,
default_liquid() -> crate::app::wallets::WalletKind { WalletKind::Liquid }) and
change the field attribute to #[serde(default = "default_liquid")] on
default_lightning_backend; alternatively, if you want every default across the
code to be Liquid, change WalletKind::default() in types.rs to return
WalletKind::Liquid — if you choose the serde-default approach, ensure
new_with_id still explicitly sets Spark for newly created cubes as before.

In `@coincube-gui/src/app/state/global_home.rs`:
- Around line 1122-1123: The filter currently skips any payment whose
payment.status is not DomainPaymentStatus::Pending, which wrongly excludes
in-flight variants; update the check in the loop that computes pending totals so
that Created and WaitingFeeAcceptance are treated as pending too (i.e., only
continue/skip for terminal states, or change the matches! to allow Pending |
Created | WaitingFeeAcceptance). Locate the conditional using
matches!(payment.status, DomainPaymentStatus::Pending) and modify it to include
DomainPaymentStatus::Created and DomainPaymentStatus::WaitingFeeAcceptance in
the set of statuses counted as pending for
restore_pending_liquid_to_vault_transfer and the pending totals calculation.

In `@coincube-gui/src/app/state/liquid/receive.rs`:
- Around line 23-35: usdt_amount_minor currently treats a payment as USDt only
when asset_info is Some; change it so the USDt check is based solely on asset_id
== usdt_id and return a sensible minor-unit value even if asset_info is None:
inside usdt_amount_minor match DomainPaymentDetails::LiquidAsset where asset_id
== usdt_id, return Some(amount_minor) when asset_info.is_some(), otherwise
return a fallback (e.g. the payment's amount_sat converted/used as the
minor-unit fallback) instead of None; alternatively split responsibilities by
adding a separate is_usdt predicate (asset_id == usdt_id) and keep this helper
only extracting minor amount with a fallback so callers that check is_some() do
not lose USDt payments.

In `@coincube-gui/src/app/state/spark/overview.rs`:
- Around line 58-69: The view() status selection wrongly prioritizes snapshot
over error, showing stale Connected(...) after a get_info() failure; fix by
either clearing self.snapshot when an error is set (e.g., in the error-handling
path that sets self.error in get_info()) or change view() to check self.error
before self.snapshot so Error(...) is returned first; apply the same fix to the
analogous logic around the code referenced at lines 141-147 (the other status
derivation).

In `@coincube-gui/src/app/state/spark/receive.rs`:
- Around line 273-299: The PaymentReceived handler
(SparkReceiveMessage::PaymentReceived) currently treats any event lacking a
BOLT11 as a match (matches_invoice => true), which allows unrelated sends to
mark the panel as received; update the handler to reject non-positive amounts
(ignore events where amount_sat <= 0) and additionally require an explicit
receive correlation when bolt11 is absent (e.g., check an explicit
direction/receive flag on the event or a receive-specific correlation field)
before setting self.displayed_invoice = None / self.qr_data = None and
transitioning to SparkReceivePhase::Received { amount_sat }; keep the existing
exact-match logic for when both self.displayed_invoice and bolt11 are Some(...)
so BOLT11 correlation still works.
- Around line 185-205: The handlers for SparkReceiveMessage::MethodSelected,
AmountInputChanged, and DescriptionInputChanged reset UI state but do not cancel
or version-guard the in-flight receive_* task, so a late
GenerateSucceeded/GenerateFailed can overwrite the new inputs; add the same
request-token/version guard used in the send flow: when changing
method/amount/description increment or rotate the receive request token/version
(or cancel the existing receive task) and ensure GenerateSucceeded and
GenerateFailed only apply their results if the token/version matches the current
one (referencing the receive_* task, GenerateSucceeded, GenerateFailed, and the
Message variants MethodSelected/AmountInputChanged/DescriptionInputChanged).

In `@coincube-gui/src/app/state/spark/send.rs`:
- Around line 115-124: The prepare task results (PrepareSucceeded/PrepareFailed)
can apply after the user edits inputs; add a monotonic prepare_version counter
to the Spark send state, bump it whenever DestinationInputChanged or
AmountInputChanged (and before launching resolve_and_prepare), pass the current
version into the spawned prepare task's completion message, and when handling
PrepareSucceeded/PrepareFailed ignore any result whose version does not equal
the current state.prepare_version; apply the same pattern for the other
prepare-launching branch (the code around lines 162-180) and ensure
ConfirmRequested only proceeds if the state.phase and prepare_version still
match the expected values.

In `@coincube-gui/src/app/state/spark/settings.rs`:
- Around line 216-264: Capture the pre-toggle value and return it with the RPC
result so the rollback can restore that exact state: in StableBalanceToggled,
save let prev = self.stable_balance_active before changing
self.stable_balance_active, then change the Task::perform payload to return
(result, prev) (i.e., async move { (backend.set_stable_balance(enabled).await,
prev) }) and update the message variant SparkSettingsMessage::StableBalanceSaved
to carry the previous value (e.g., Result<(bool, Option<bool>), String> or add a
new payload field) so StableBalanceSaved(result, prev) handler can set
self.stable_balance_saving = false and on Err restore self.stable_balance_active
= prev rather than flipping the current value; update all construction sites of
StableBalanceSaved in the StableBalanceToggled success/error branches
accordingly (use backend.set_stable_balance, StableBalanceToggled,
StableBalanceSaved, self.stable_balance_active, self.stable_balance_saving to
locate changes).

In `@coincube-gui/src/app/view/spark/receive.rs`:
- Around line 267-274: The short_error function currently slices the input
string by byte index (using &err[..MAX]) which can panic for multi-byte UTF-8
characters; change the truncation to operate on Unicode scalar values (chars)
instead of bytes — e.g., count up to MAX characters using err.char_indices() or
err.chars().take(MAX).collect::<String>() and then append the ellipsis when
truncated; keep the MAX constant as the character limit and ensure the function
returns the full string when its character count is <= MAX.

In `@coincube-gui/src/app/view/spark/transactions.rs`:
- Around line 60-64: Update the empty-state copy in the
SparkTransactionsStatus::Loaded(payments) if payments.is_empty() branch (the
Column::new() push that calls p1_regular) so it no longer references "once the
Send / Receive panels land"; replace that string with text that reflects the
panels are available now (for example: "No payments yet. Incoming and outgoing
payments will appear here when you send or receive funds."). Ensure the new
message is used in the same place where p1_regular(...) is currently called.

In `@coincube-gui/src/gui/tab.rs`:
- Around line 367-370: The installer path recreates the app with
app::App::new_without_wallet passing None for the Spark backend, which drops an
existing spark_backend when backing out of vault setup; update the Installer
struct to carry the existing spark_backend (similar to how it already carries
breez_client) and when constructing the app in the vault flow pass
Installer.spark_backend (or clone) into app::App::new_without_wallet instead of
None so the Spark backend is preserved; ensure all places that construct
Installer initialize and propagate spark_backend accordingly.

In `@coincube-gui/src/services/lnurl/stream.rs`:
- Around line 321-344: The Spark branch currently trusts event.description
without verifying it against event.description_hash; compute the SHA-256 hash of
the UTF-8 bytes of event.description and compare it (in the same encoding/format
used elsewhere for event.description_hash) before calling receive_bolt11 in the
use_spark path (symbols: use_spark, event.description, event.description_hash,
receive_bolt11, WalletKind::Spark, BOLT11_MAX_DESCRIPTION_BYTES); if the hash
does not match, treat it as a mismatch and fall back to the non-Spark path
(i.e., do not call spark.receive_bolt11 and proceed to the else/liquid handling)
or return an error, so the minted invoice’s d tag cannot diverge from the
validated metadata hash.

In `@coincube-spark-bridge/src/server.rs`:
- Around line 63-85: The sweep task keeps the ServerState alive and prevents
writer shutdown; replace the strong Arc usage with a Weak reference so the sweep
doesn’t extend ServerState lifetime: create let sweep_state =
Arc::downgrade(&state) and inside the loop attempt sweep_state.upgrade() — if
upgrade() returns None break the loop, otherwise call sweep_expired_prepares
with the upgraded Arc; additionally ensure any extra event_tx clones owned by
ServerState are dropped (or explicitly closed) before awaiting writer_task in
run() so rx.recv() can observe channel closure.

In `@docs/WALLETS.md`:
- Around line 7-31: The fenced code block in docs/WALLETS.md currently has a
bare opening fence which triggers markdownlint MD040; update the opening fence
to specify a language (e.g., change the backticks starting the tree block to
```text) so the tree layout block is explicitly marked as text; locate the
fenced block that contains the directory tree under coincube-gui/src/app/ and
modify only the opening fence to include the language identifier (leave the
block contents and closing fence unchanged).

---

Outside diff comments:
In `@coincube-gui/src/app/state/liquid/send.rs`:
- Around line 790-833: The USDt classification must use the asset_id check
instead of relying on asset_info presence: set is_usdt_payment by matching
payment.details against DomainPaymentDetails::LiquidAsset { asset_id, .. } and
comparing asset_id == usdt_id; compute usdt_amount_minor separately as
asset_info.as_ref().map(|i| i.amount_minor) only when asset_info exists; keep
amount = Amount::from_sat(usdt_amount_minor.unwrap_or(payment.amount_sat)); keep
fiat_amount = None when is_usdt_payment else use fiat_converter.convert(amount);
and adjust the desc match to detect USDt by asset_id (or is_usdt_payment) but
avoid depending on asset_info for choosing the "USDt Transfer" fallback.

In `@coincube-gui/src/gui/tab.rs`:
- Around line 975-984: The create_app_with_remote_backend function currently
hardcodes None for the Spark argument when calling App::new, which prevents
Spark loaded by BreezClientLoadedAfterPin from being visible; change the
signature of create_app_with_remote_backend to accept the Spark (or
Option<SparkType>) parameter and forward that value into app::App::new instead
of None, then update all call sites (including where BreezClientLoadedAfterPin
returns a loaded Breez client/PIN flow) to pass the loaded Spark through; ensure
the type matches the App::new parameter and adjust related declarations where
the function is defined/used (also update the other occurrence around lines
1114-1115) so remote-backend users receive the loaded Spark after login.

In `@coincube-gui/src/loader.rs`:
- Around line 858-865: The sync function is blocking the tokio executor by
calling std::thread::sleep; replace that call in async fn sync(...) with an
asynchronous sleep using
tokio::time::sleep(std::time::Duration::from_secs(1)).await so the task yields
instead of parking the worker thread; keep the existing daemon.get_info().await
return behavior and add the tokio::time::sleep call only when sleep == true.

In `@coincube-gui/src/services/lnurl/stream.rs`:
- Around line 21-42: The Hash impl for LnurlStreamData currently only hashes
token and retries, so changes to routing inputs are ignored; update the Hash
implementation for LnurlStreamData to also incorporate the routing-related
fields used by create_stream/handle_invoice_request: include preferred
(WalletKind) and the spark_backend identity (e.g., hash spark_backend.is_some()
or the Arc pointer/inner identity) so that changes to default_lightning_backend
or the Spark bridge presence will change the stream's hash and force Iced to
recreate the SSE task.

---

Nitpick comments:
In `@coincube-gui/src/app/view/liquid/send.rs`:
- Around line 1881-2041: Delete the parked legacy toggle by removing the entire
_old_send_asset_toggle function (including the leading NOTE comment and the
#[allow(dead_code)] attribute) from the file; ensure you also remove any
leftover references to _old_send_asset_toggle and its symbol (fn
_old_send_asset_toggle, SendAsset variants used only there) so the
compiler/warnings stay clean, and run a quick build to confirm no callers remain
(e.g. search for _old_send_asset_toggle and LiquidSendMessage::PresetAsset
usages that were only for this old toggle).

In `@coincube-spark-bridge/Cargo.toml`:
- Around line 22-23: Add a brief comment next to the Cargo.toml lints override
explaining why mismatched_lifetime_syntaxes is set to "allow" (e.g., due to
upstream/generated code or a specific dependency causing unavoidable lifetime
syntax mismatches). Locate the [lints.rust] section and the
mismatched_lifetime_syntaxes entry and insert a one-line rationale referencing
the upstream crate or codegen that necessitates this override so future
maintainers understand the exception.

In `@docs/SPARK_WALLET.md`:
- Around line 13-25: The fenced ASCII diagram (the triple backticks surrounding
the diagram in the docs/SPARK_WALLET.md file) is missing a language specifier;
update the opening fence from ``` to ```text (or an empty specifier) so the
block is marked as plain text and satisfies the markdown linter—edit the code
block that contains the ASCII diagram (the block starting with the triple
backticks above the diagram) to include the specifier.
🪄 Autofix (Beta)

Fix all unresolved CodeRabbit comments on this PR:

  • Push a commit to this branch (recommended)
  • Create a new PR with the fixes

ℹ️ Review info
⚙️ Run configuration

Configuration used: defaults

Review profile: CHILL

Plan: Pro

Run ID: c88001fc-28c4-49b5-ad66-7c553c8b2633

📥 Commits

Reviewing files that changed from the base of the PR and between de3e18d and 72ca626.

⛔ Files ignored due to path filters (2)
  • Cargo.lock is excluded by !**/*.lock
  • coincube-spark-bridge/Cargo.lock is excluded by !**/*.lock
📒 Files selected for processing (71)
  • .env.example
  • Cargo.toml
  • coincube-gui/Cargo.toml
  • coincube-gui/src/app/breez_liquid/assets.rs
  • coincube-gui/src/app/breez_liquid/client.rs
  • coincube-gui/src/app/breez_liquid/config.rs
  • coincube-gui/src/app/breez_liquid/mod.rs
  • coincube-gui/src/app/breez_spark/assets.rs
  • coincube-gui/src/app/breez_spark/client.rs
  • coincube-gui/src/app/breez_spark/config.rs
  • coincube-gui/src/app/breez_spark/mod.rs
  • coincube-gui/src/app/cache.rs
  • coincube-gui/src/app/menu.rs
  • coincube-gui/src/app/message.rs
  • coincube-gui/src/app/mod.rs
  • coincube-gui/src/app/settings/mod.rs
  • coincube-gui/src/app/state/connect/cube.rs
  • coincube-gui/src/app/state/connect/mod.rs
  • coincube-gui/src/app/state/global_home.rs
  • coincube-gui/src/app/state/liquid/overview.rs
  • coincube-gui/src/app/state/liquid/receive.rs
  • coincube-gui/src/app/state/liquid/send.rs
  • coincube-gui/src/app/state/liquid/settings.rs
  • coincube-gui/src/app/state/liquid/sideshift_receive.rs
  • coincube-gui/src/app/state/liquid/sideshift_send.rs
  • coincube-gui/src/app/state/liquid/transactions.rs
  • coincube-gui/src/app/state/mod.rs
  • coincube-gui/src/app/state/spark/mod.rs
  • coincube-gui/src/app/state/spark/overview.rs
  • coincube-gui/src/app/state/spark/receive.rs
  • coincube-gui/src/app/state/spark/send.rs
  • coincube-gui/src/app/state/spark/settings.rs
  • coincube-gui/src/app/state/spark/transactions.rs
  • coincube-gui/src/app/state/vault/export.rs
  • coincube-gui/src/app/view/buysell/mavapay/mod.rs
  • coincube-gui/src/app/view/buysell/panel.rs
  • coincube-gui/src/app/view/global_home.rs
  • coincube-gui/src/app/view/liquid/overview.rs
  • coincube-gui/src/app/view/liquid/receive.rs
  • coincube-gui/src/app/view/liquid/send.rs
  • coincube-gui/src/app/view/liquid/sideshift_send.rs
  • coincube-gui/src/app/view/liquid/transactions.rs
  • coincube-gui/src/app/view/message.rs
  • coincube-gui/src/app/view/mod.rs
  • coincube-gui/src/app/view/spark/mod.rs
  • coincube-gui/src/app/view/spark/overview.rs
  • coincube-gui/src/app/view/spark/receive.rs
  • coincube-gui/src/app/view/spark/send.rs
  • coincube-gui/src/app/view/spark/settings.rs
  • coincube-gui/src/app/view/spark/transactions.rs
  • coincube-gui/src/app/wallets/liquid.rs
  • coincube-gui/src/app/wallets/mod.rs
  • coincube-gui/src/app/wallets/registry.rs
  • coincube-gui/src/app/wallets/spark.rs
  • coincube-gui/src/app/wallets/types.rs
  • coincube-gui/src/export.rs
  • coincube-gui/src/gui/tab.rs
  • coincube-gui/src/installer/mod.rs
  • coincube-gui/src/launcher.rs
  • coincube-gui/src/loader.rs
  • coincube-gui/src/services/connect/login.rs
  • coincube-gui/src/services/lnurl/mod.rs
  • coincube-gui/src/services/lnurl/stream.rs
  • coincube-spark-bridge/Cargo.toml
  • coincube-spark-bridge/src/main.rs
  • coincube-spark-bridge/src/sdk_adapter.rs
  • coincube-spark-bridge/src/server.rs
  • coincube-spark-protocol/Cargo.toml
  • coincube-spark-protocol/src/lib.rs
  • docs/SPARK_WALLET.md
  • docs/WALLETS.md

Comment thread .env.example Outdated
Comment thread coincube-gui/src/app/breez_spark/client.rs Outdated
Comment thread coincube-gui/src/app/breez_spark/client.rs
Comment thread coincube-gui/src/app/breez_spark/config.rs Outdated
Comment on lines +178 to +183
/// Which backend should fulfill incoming Lightning Address invoices
/// for this cube. Starts as `Liquid` (backwards-compatible default
/// for existing cubes); Phase 5 flips the default to `Spark` and
/// adds a per-cube override in Settings.
#[serde(default)]
pub default_lightning_backend: crate::app::wallets::WalletKind,
Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

⚠️ Potential issue | 🟡 Minor

Documentation states incorrect default value.

The doc comment states the default is Liquid for backwards compatibility, but WalletKind::default() in types.rs is Spark. For existing cubes where the field is absent, #[serde(default)] will use Spark, not Liquid.

If backwards compatibility requires Liquid as the default for existing cubes, you need to either:

  1. Change WalletKind::default() to Liquid, or
  2. Use a custom default function: #[serde(default = "default_liquid")]
🔧 Option: Custom default for backwards compatibility

If Liquid should be the default for deserialization (existing cubes):

+fn default_lightning_backend() -> crate::app::wallets::WalletKind {
+    crate::app::wallets::WalletKind::Liquid
+}
+
 /// Which backend should fulfill incoming Lightning Address invoices
-/// for this cube. Starts as `Liquid` (backwards-compatible default
-/// for existing cubes); Phase 5 flips the default to `Spark` and
-/// adds a per-cube override in Settings.
-#[serde(default)]
+/// for this cube. Defaults to `Liquid` for backwards compatibility
+/// with existing cubes; new cubes created in Phase 5+ default to `Spark`.
+#[serde(default = "default_lightning_backend")]
 pub default_lightning_backend: crate::app::wallets::WalletKind,

Then in new_with_id, explicitly set Spark for new cubes (which is already done at line 215).

🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.

In `@coincube-gui/src/app/settings/mod.rs` around lines 178 - 183, The doc is
wrong because serde(default) will use WalletKind::default() (currently Spark);
fix by providing an explicit deserialization default that returns Liquid: add a
function (e.g., default_liquid() -> crate::app::wallets::WalletKind {
WalletKind::Liquid }) and change the field attribute to #[serde(default =
"default_liquid")] on default_lightning_backend; alternatively, if you want
every default across the code to be Liquid, change WalletKind::default() in
types.rs to return WalletKind::Liquid — if you choose the serde-default
approach, ensure new_with_id still explicitly sets Spark for newly created cubes
as before.

Comment thread coincube-gui/src/app/view/spark/transactions.rs Outdated
Comment thread coincube-gui/src/gui/tab.rs
Comment thread coincube-gui/src/services/lnurl/stream.rs
Comment thread coincube-spark-bridge/src/server.rs
Comment thread docs/WALLETS.md Outdated
Copy link
Copy Markdown

@cursor cursor bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Cursor Bugbot has reviewed your changes and found 2 potential issues.

Fix All in Cursor

❌ Bugbot Autofix is OFF. To automatically fix reported issues with cloud agents, enable autofix in the Cursor dashboard.

Reviewed by Cursor Bugbot for commit a6b27d0. Configure here.

Comment thread coincube-gui/src/app/breez_spark/client.rs
Comment thread coincube-gui/src/app/state/global_home.rs
Copy link
Copy Markdown

@coderabbitai coderabbitai bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actionable comments posted: 10

Caution

Some comments are outside the diff and can’t be posted inline due to platform limitations.

⚠️ Outside diff range comments (1)
coincube-gui/src/app/view/global_home.rs (1)

1739-1859: ⚠️ Potential issue | 🟠 Major

Hide the Spark card when Spark is unavailable.

Lines 1741-1743 say the card should disappear for cubes without a Spark signer, but Line 1858 still pushes spark_card unconditionally. With the current spark_balance defaulting to zero, non-Spark cubes will show a dead 0-sat card and expose Spark navigation that cannot work. Pass an explicit availability flag/Option through GlobalViewConfig and only render the card when the backend is actually present.

🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.

In `@coincube-gui/src/app/view/global_home.rs` around lines 1739 - 1859, The Spark
card is still pushed unconditionally (showing 0 sats and broken navigation) even
though the comment says it should be hidden when Spark is unavailable; update
the view to accept an explicit availability flag (e.g. add a has_spark: bool or
Option to GlobalViewConfig) and only construct/push spark_card (and wrap
mouse_area/on_press Message::Menu(Menu::Spark(...))) when that flag is
true/present; locate symbols spark_card, spark_card_content, spark_btc_row,
spark_balance and the final Column::new() builder and replace the unconditional
.push(spark_card) with a conditional push (or skip building spark_card entirely)
based on the new flag so non-Spark cubes do not show Spark UI or navigation.
♻️ Duplicate comments (8)
coincube-gui/src/app/state/liquid/receive.rs (1)

23-36: ⚠️ Potential issue | 🟡 Minor

USDt detection still relies on asset_info presence.

The usdt_amount_minor helper returns None when asset_info is None, even if asset_id == usdt_id. This means a USDt payment lacking asset_info metadata would be misclassified as non-USDt throughout this file (filtering at line 619, celebration at line 641, display at line 673).

Consider separating the USDt predicate from the amount extraction:

🔧 Proposed fix
-fn usdt_amount_minor(details: &DomainPaymentDetails, usdt_id: &str) -> Option<u64> {
+/// Returns true if this is a USDt payment (asset_id matches usdt_id).
+fn is_usdt_payment(details: &DomainPaymentDetails, usdt_id: &str) -> bool {
+    matches!(
+        details,
+        DomainPaymentDetails::LiquidAsset { asset_id, .. }
+            if !usdt_id.is_empty() && asset_id == usdt_id
+    )
+}
+
+/// Extract the minor-unit USDt amount, falling back to payment.amount_sat if asset_info is absent.
+fn usdt_amount_minor(details: &DomainPaymentDetails, usdt_id: &str, fallback_sat: u64) -> Option<u64> {
     match details {
         DomainPaymentDetails::LiquidAsset {
             asset_id,
             asset_info,
             ..
         } if !usdt_id.is_empty() && asset_id == usdt_id => {
-            asset_info.as_ref().map(|i| i.amount_minor)
+            Some(asset_info.as_ref().map(|i| i.amount_minor).unwrap_or(fallback_sat))
         }
         _ => None,
     }
 }
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.

In `@coincube-gui/src/app/state/liquid/receive.rs` around lines 23 - 36, The
current usdt_amount_minor helper ties USDt detection to asset_info presence,
causing payments with asset_id == usdt_id but missing asset_info to be
misclassified; add a separate predicate (e.g., fn is_usdt(details:
&DomainPaymentDetails, usdt_id: &str) -> bool) that returns true when
DomainPaymentDetails::LiquidAsset { asset_id, .. } has asset_id == usdt_id
regardless of asset_info, and keep usdt_amount_minor only responsible for
extracting amount (returning Option<u64> from asset_info.as_ref().map(|i|
i.amount_minor)); then update all places that currently test for USDt to use
is_usdt instead of relying on usdt_amount_minor (references: usdt_amount_minor
and DomainPaymentDetails::LiquidAsset).
coincube-gui/src/app/state/global_home.rs (1)

1179-1180: ⚠️ Potential issue | 🟡 Minor

Include all in-flight statuses in the pending totals.

Line 1179 still counts only DomainPaymentStatus::Pending, so Created and WaitingFeeAcceptance transfers fall out of the home-page pending amounts even though they are still active. Treat the terminal statuses as the exclusion set, or explicitly include all non-terminal in-flight variants here.

🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.

In `@coincube-gui/src/app/state/global_home.rs` around lines 1179 - 1180, The
current check only counts DomainPaymentStatus::Pending in the pending totals,
excluding other in-flight statuses like Created and WaitingFeeAcceptance; update
the condition in the block that examines payment.status (the if
!matches!(payment.status, DomainPaymentStatus::Pending) continue; line) to
instead treat terminal statuses as the exclusion set or explicitly match all
non-terminal variants (e.g., include Created and WaitingFeeAcceptance alongside
Pending) so that all in-flight transfers contribute to the home-page pending
amounts; if there is an existing helper like is_terminal or is_final, prefer
using that to skip only terminal statuses.
coincube-gui/src/gui/tab.rs (1)

341-351: ⚠️ Potential issue | 🟠 Major

Some transitions still discard a loaded Spark backend.

These branches overwrite spark_backend with None, so Spark disappears after vault-setup flows and for remote-backend cubes even when the PIN task already initialized it successfully. Thread the existing backend through these constructors the same way breez_client is preserved.

Also applies to: 408-417, 787-800, 1115-1117

🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.

In `@coincube-gui/src/gui/tab.rs` around lines 341 - 351, The Loader::new calls in
tab.rs are discarding an existing Spark backend by passing None (e.g., the call
that passes i.breez_client.clone() followed by None); instead thread the current
spark_backend Option through these constructors the same way breez_client is
preserved: locate each Loader::new invocation (including the other occurrences
referenced) and replace the final None argument with the existing spark_backend
variable (cloned or referenced as appropriate), ensuring the Loader::new
signature is used consistently so an already-initialized Spark backend is not
overwritten.
coincube-gui/src/app/state/spark/settings.rs (1)

151-187: ⚠️ Potential issue | 🟠 Major

Rollback needs the pre-toggle value, not !current.

If UserSettingsLoaded or a reload updates stable_balance_active while the RPC is in flight, flipping !current can restore the wrong value on failure. Capture the previous state when dispatching set_stable_balance and return that with StableBalanceSaved.

🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.

In `@coincube-gui/src/app/state/spark/settings.rs` around lines 151 - 187, When
dispatching crate::app::view::SparkSettingsMessage::StableBalanceToggled,
capture the previous self.stable_balance_active (e.g. let prev =
self.stable_balance_active) and include it in the async Task result so
StableBalanceSaved carries the previous value along with the success/err; change
the Task::perform closure to return a tuple (Result<(), E>, Option<bool>) or
otherwise encode the previous state, and update the StableBalanceSaved match arm
to on Err use that provided previous value to restore stable_balance_active
(instead of computing !current) and set SparkSettingsStatus::Error with the
error string. Ensure references to set_stable_balance, StableBalanceToggled,
StableBalanceSaved, and stable_balance_active are updated accordingly.
coincube-gui/src/app/state/spark/overview.rs (1)

73-83: ⚠️ Potential issue | 🟠 Major

Prioritize error over cached snapshot.

A failed refresh still renders Connected(...) whenever self.snapshot is set, so the panel can keep showing stale balance data after get_info() fails. Check self.error before self.snapshot, or clear self.snapshot in the error branch.

🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.

In `@coincube-gui/src/app/state/spark/overview.rs` around lines 73 - 83, The
status computation currently prefers snapshot over error, causing stale
Connected(...) UI when get_info() fails; modify the logic in the block that
builds `status` so it checks `self.error` before `self.snapshot` (i.e., if let
Some(err) = &self.error => SparkStatus::Error(err.clone()) comes before the `if
let Some(snapshot)` branch) or ensure that wherever `self.error` is set (e.g.,
after `get_info()` fails) you also clear `self.snapshot` so
`SparkStatus::Connected(snapshot.clone())` cannot be chosen when an error
exists. Target the status construction logic around `SparkStatus::Unavailable`,
`SparkStatus::Loading`, `SparkStatus::Connected`, and `SparkStatus::Error` to
implement the change.
coincube-gui/src/app/view/spark/receive.rs (1)

268-274: ⚠️ Potential issue | 🟡 Minor

Truncate error text on char boundaries.

&err[..MAX] slices bytes, so a multibyte UTF-8 error string can panic while rendering the retry row. Truncate with char_indices() or chars().take(MAX) instead.

🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.

In `@coincube-gui/src/app/view/spark/receive.rs` around lines 268 - 274, The
short_error function currently slices the string by bytes using &err[..MAX],
which can panic on multibyte UTF-8 characters; change short_error to truncate on
character boundaries by taking the first MAX chars (e.g., using
err.chars().take(MAX).collect::<String>() or using char_indices() to find a safe
byte index) and then append the ellipsis if truncated so the function never
slices mid-character.
coincube-gui/src/app/state/spark/send.rs (1)

115-179: ⚠️ Potential issue | 🔴 Critical

Version-guard async prepare results.

Editing the destination or amount resets the phase locally, but the in-flight resolve_and_prepare() task still resolves into PrepareSucceeded / PrepareFailed. A late completion can resurrect a handle for the old destination and let ConfirmRequested send the wrong payment. Add a monotonic prepare token/version and ignore completions that do not match the latest form state.

Also applies to: 180-190

🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.

In `@coincube-gui/src/app/state/spark/send.rs` around lines 115 - 179, The async
prepare task can complete after the user edits inputs and must be ignored if
stale: add a monotonic prepare token (e.g. prepare_version: u64) to the
SparkSend state, increment it whenever destination_input or amount_input changes
(in the SparkSendMessage::DestinationInputChanged and ::AmountInputChanged
handlers and when entering Preparing), capture the current token in the async
Task that calls resolve_and_prepare(…) and include that token in the returned
message payload, and when handling SparkSendMessage::PrepareSucceeded and
::PrepareFailed compare the returned token against the current prepare_version
before setting self.phase (only apply results when they match); likewise ensure
ConfirmRequested checks the current prepared version if needed.
coincube-gui/src/app/state/spark/receive.rs (1)

183-249: ⚠️ Potential issue | 🟠 Major

Ignore generate results from superseded form state.

Changing method, amount, or description resets the panel immediately, but the in-flight receive_bolt11 / receive_onchain task still resolves into GenerateSucceeded / GenerateFailed. A late completion can replace the new form with an invoice/address generated for the old inputs. Add a request token/version and discard stale completions.

Also applies to: 252-266

🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.

In `@coincube-gui/src/app/state/spark/receive.rs` around lines 183 - 249, The
generate completion handlers can overwrite a newer form state because in-flight
tasks from previous inputs still dispatch GenerateSucceeded/GenerateFailed; add
a monotonically-incremented request token field on the SparkReceive state (e.g.,
generate_req_id: u64), increment it when handling
SparkReceiveMessage::GenerateRequested and capture the current token in both
Task::perform async closures for backend.receive_bolt11 and
backend.receive_onchain, then when mapping the result into
SparkReceiveMessage::GenerateSucceeded/GenerateFailed include the token and, in
the Update handling of those messages, only apply the result if the token
matches self.generate_req_id (otherwise ignore as stale). Update both the Bolt11
and Onchain branches to capture and propagate the token, and ensure other reset
paths (MethodSelected, AmountInputChanged, DescriptionInputChanged) also
increment or invalidate the token so late completions are discarded.
🧹 Nitpick comments (4)
coincube-ui/src/color.rs (1)

54-54: Prefer aliasing WARN_ORANGE to DARK_ORANGE to prevent token drift.

Line 54 repeats the same literal used by DARK_ORANGE. Referencing the existing constant keeps this mapping synchronized automatically.

♻️ Proposed refactor
-pub const WARN_ORANGE: Color = iced::color!(0xCA6719); // ~4.5:1 (same as DARK_ORANGE)
+pub const WARN_ORANGE: Color = DARK_ORANGE; // ~4.5:1
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.

In `@coincube-ui/src/color.rs` at line 54, WARN_ORANGE duplicates the same literal
as DARK_ORANGE which can drift; change WARN_ORANGE to alias DARK_ORANGE instead
of repeating the hex literal by assigning WARN_ORANGE = DARK_ORANGE (keep the
same type Color and const declaration), so any future changes to DARK_ORANGE
automatically apply to WARN_ORANGE; update the const named WARN_ORANGE to
reference DARK_ORANGE rather than using iced::color!(0xCA6719).
coincube-gui/src/app/state/liquid/send.rs (1)

789-833: Consider extracting the USDt detection helper.

The USDt amount extraction pattern (matching DomainPaymentDetails::LiquidAsset and extracting asset_info.amount_minor) is duplicated across send.rs, overview.rs, and receive.rs. The receive.rs file already has a local usdt_amount_minor() helper — consider promoting it to a shared location (e.g., wallets/types.rs or breez_liquid/assets.rs) to reduce duplication.

🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.

In `@coincube-gui/src/app/state/liquid/send.rs` around lines 789 - 833, Extract
the USDt-detection logic into a shared helper like fn usdt_amount_minor(details:
&DomainPaymentDetails, usdt_id: &str) -> Option<u64> (or similar) placed in a
common module (e.g., wallets types or breez_liquid assets), then replace the
duplicated matches in send.rs/overview.rs/receive.rs with calls to that helper;
update send.rs to use the helper to set is_usdt_payment, amount, and
fiat_amount, and keep the description branch using the helper result instead of
re-matching DomainPaymentDetails::LiquidAsset.
coincube-gui/src/app/breez_spark/client.rs (2)

851-861: Unreachable break after pending::<()>().await.

std::future::pending::<()>().await never resolves, making the break at line 857 dead code. The intent (parking the task until the subscription is dropped) is correct, but the break is misleading.

🧹 Remove unreachable break
                     Err(broadcast::error::RecvError::Closed) => {
                         // Sender dropped — the SparkClient is gone.
                         // Park the task forever so iced keeps the
                         // Subscription id alive until the parent
                         // re-instantiates the state.
                         std::future::pending::<()>().await;
-                        break;
                     }
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.

In `@coincube-gui/src/app/breez_spark/client.rs` around lines 851 - 861, In the
match arm handling broadcast::error::RecvError::Closed inside the SparkClient
subscription loop, remove the unreachable break that follows
std::future::pending::<()>().await (the break after the pending await is dead
code); leave the pending await to park the task and optionally adjust the
comment to explain the intent so the code clearly reflects that the task is
intentionally parked until the subscription is dropped.

668-674: Misleading comment: the task doesn't drain requests on stdin failure.

The warn message and comment imply remaining requests are drained, but the task simply breaks and exits. Requests already in the pending map won't be failed until the reader task also exits and drains them (lines 729-750). If stdin breaks but stdout remains open (unlikely but possible), callers could wait the full 30s timeout.

Consider either:

  1. Updating the comment to reflect actual behavior ("stopping, bridge dead"), or
  2. Having the writer task signal the reader to exit (e.g., via a shutdown channel) so pending requests fail faster.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.

In `@coincube-gui/src/app/breez_spark/client.rs` around lines 668 - 674, The warn
message and comment in the writer task (the warn!("Spark bridge writer: stdin
closed, draining remaining requests") and surrounding comment) are misleading
because the writer simply breaks and does not drain the pending map; update
either the comment/message to accurately say the writer is stopping and the
bridge is dead, or implement a shutdown signal so the writer notifies the reader
to exit early: add a shutdown channel (e.g., tokio::sync::watch or oneshot),
have the writer send the shutdown when stdin is closed and adjust the reader
task to select on that shutdown signal and then fail pending requests in the
pending map immediately; ensure you update the warn log text and reference the
writer task, reader task, and pending map when making changes.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.

Inline comments:
In `@coincube-gui/src/app/breez_spark/mod.rs`:
- Around line 53-57: The match in breez_spark::mod.rs currently allows
Network::Regtest to proceed which leads to downstream handshake failures; update
the network check in the match over network (the branch handling
Network::Bitcoin | Network::Regtest) to reject Regtest early by returning
Err(SparkLoadError::NetworkNotSupported(network)) for anything other than
Network::Bitcoin so that Network::Regtest is treated like other unsupported
networks and the load fails fast.

In `@coincube-gui/src/app/state/global_home.rs`:
- Around line 1134-1142: The Err(e) branch currently emits
Message::View(view::Message::Home(HomeMessage::SparkBalanceUpdated(Amount::ZERO)))
which incorrectly zeros the cached Spark balance on transient get_info failures;
change the Err(e) handling in that match to avoid dispatching
SparkBalanceUpdated(Amount::ZERO) — either do not emit any balance-update
message (so the cached balance remains unchanged) or emit a dedicated
error/no-op message (e.g., HomeMessage::SparkBalanceFetchFailed or a no-op
HomeMessage variant) instead of using Amount::ZERO; update the Err(e) arm that
contains tracing::warn! to only log the error and return the non-updating/no-op
message.

In `@coincube-gui/src/app/state/settings/lightning.rs`:
- Around line 77-99: The code always returns Some(settings) from the
update_settings_file closure even when the cube lookup fails, so the outer
result becomes Ok(()) and the UI emits Message::SettingsSaved; change the
closure so that when settings.cubes.iter_mut().find(|c| c.id == cube_id) is None
you do not return Some(settings) but instead return None (or otherwise propagate
an error) so update_settings_file will fail and the match will take the Err
branch (showing the failure message) rather than emitting
Message::SettingsSaved; target the closure passed to update_settings_file around
the cube lookup and default_lightning_backend assignment and replace the
logging-only path with a non-Some return.

In `@coincube-gui/src/app/state/spark/overview.rs`:
- Around line 119-123: The reload flow drops identity_pubkey because get_info()
is used but view::SparkOverviewMessage::DataLoaded only carries balance and
recent_payments; update the DataLoaded payload (and its construction in the
match arm where get_info() and list are Ok) to include identity_pubkey from info
(e.g., pass info.identity_pubkey into DataLoaded) and propagate it through
SparkBalanceSnapshot, or if the view truly does not need identity_pubkey, remove
identity_pubkey from the SparkBalanceSnapshot and its usages; change the
DataLoaded variant and any constructors/consumers (e.g., where DataLoaded is
matched/handled and SparkBalanceSnapshot is created) accordingly so the pubkey
is not lost on first load.

In `@coincube-gui/src/app/state/spark/receive.rs`:
- Around line 255-258: GenerateSucceeded currently sets displayed_invoice for
all receive methods causing PaymentReceived to reject on-chain receipts because
PaymentReceived only treats bolt11 == None as valid when displayed_invoice is
None; change GenerateSucceeded (the branch that sets self.displayed_invoice) to
only populate self.displayed_invoice when the payload is a BOLT11 invoice (i.e.,
when ok.payment_request is a bolt11 string), leaving displayed_invoice as None
for on-chain/deposit-address flows, or alternatively add an explicit correlation
in PaymentReceived to accept matches where the event carries an on-chain deposit
address; update both spots mentioned (the block that sets self.displayed_invoice
around lines shown and the similar block at 285-289) to implement the
BOLT11-only assignment and ensure PaymentReceived's matching logic still uses
displayed_invoice and bolt11 consistently.

In `@coincube-gui/src/app/view/spark/settings.rs`:
- Around line 83-85: stable_balance_card is always invoked with a hardcoded
`true`, so the stable-balance toggle remains enabled even when the bridge is
unreachable (SparkSettingsStatus::Error(_)); change the call to compute the
enabled flag from the bridge status (e.g. match on `self.status` or add an
`is_reachable()` helper) and pass that boolean instead of `true`, ensuring the
toggle (and thus calls to set_stable_balance) are disabled when `self.status`
indicates an error/unreachable bridge; update the invocation of
`stable_balance_card(self.stable_balance_active, self.stable_balance_saving,
true)` to use the computed reachable boolean derived from `self.status`.

In `@coincube-spark-bridge/src/main.rs`:
- Around line 33-35: The smoke-test path still requires BREEZ_API_KEY directly
instead of using the new BREEZ_SPARK_API_KEY fallback; update the code that
reads the Spark API key (the places that currently read BREEZ_API_KEY for
--smoke-test, referenced around the arg declaration and the smoke-test handling
near lines 69-76) to first try env var BREEZ_SPARK_API_KEY and if missing fall
back to BREEZ_API_KEY, and use that resulting value wherever the smoke-test
checks or passes the Spark API key (e.g., the variables/params named like
spark_api_key or breez_api_key used in the smoke-test logic).

In `@docs/SPARK_WALLET.md`:
- Around line 13-25: The fenced ASCII diagram block in docs/SPARK_WALLET.md is
missing a language hint which triggers markdownlint MD040; update the opening
fence for the diagram (the triple-backtick block around the coincube-gui /
coincube-spark-bridge diagram) to include a language hint such as "text" (i.e.,
change ``` to ```text) so the linter recognizes it as a text/code block.
- Around line 37-38: Update the Spark wallet docs to match the current code and
UI: replace references to spark_wallet_signer_fingerprint and
liquid_wallet_signer_fingerprint with cube.breez_wallet_signer_fingerprint,
change any "Spark → Settings" instructions to "Settings → Lightning" (or the new
default-backend picker location), and adjust setup/troubleshooting text that
describes how the GUI spawns the bridge to mention the code now loads both SDKs
from cube.breez_wallet_signer_fingerprint; ensure all occurrences (including the
ones noted around lines 49-52, 74-75, 88-90) are updated and that guidance about
COINCUBE_SPARK_BRIDGE_PATH and the bridge spawn behavior remains accurate with
the new loading logic.

---

Outside diff comments:
In `@coincube-gui/src/app/view/global_home.rs`:
- Around line 1739-1859: The Spark card is still pushed unconditionally (showing
0 sats and broken navigation) even though the comment says it should be hidden
when Spark is unavailable; update the view to accept an explicit availability
flag (e.g. add a has_spark: bool or Option to GlobalViewConfig) and only
construct/push spark_card (and wrap mouse_area/on_press
Message::Menu(Menu::Spark(...))) when that flag is true/present; locate symbols
spark_card, spark_card_content, spark_btc_row, spark_balance and the final
Column::new() builder and replace the unconditional .push(spark_card) with a
conditional push (or skip building spark_card entirely) based on the new flag so
non-Spark cubes do not show Spark UI or navigation.

---

Duplicate comments:
In `@coincube-gui/src/app/state/global_home.rs`:
- Around line 1179-1180: The current check only counts
DomainPaymentStatus::Pending in the pending totals, excluding other in-flight
statuses like Created and WaitingFeeAcceptance; update the condition in the
block that examines payment.status (the if !matches!(payment.status,
DomainPaymentStatus::Pending) continue; line) to instead treat terminal statuses
as the exclusion set or explicitly match all non-terminal variants (e.g.,
include Created and WaitingFeeAcceptance alongside Pending) so that all
in-flight transfers contribute to the home-page pending amounts; if there is an
existing helper like is_terminal or is_final, prefer using that to skip only
terminal statuses.

In `@coincube-gui/src/app/state/liquid/receive.rs`:
- Around line 23-36: The current usdt_amount_minor helper ties USDt detection to
asset_info presence, causing payments with asset_id == usdt_id but missing
asset_info to be misclassified; add a separate predicate (e.g., fn
is_usdt(details: &DomainPaymentDetails, usdt_id: &str) -> bool) that returns
true when DomainPaymentDetails::LiquidAsset { asset_id, .. } has asset_id ==
usdt_id regardless of asset_info, and keep usdt_amount_minor only responsible
for extracting amount (returning Option<u64> from asset_info.as_ref().map(|i|
i.amount_minor)); then update all places that currently test for USDt to use
is_usdt instead of relying on usdt_amount_minor (references: usdt_amount_minor
and DomainPaymentDetails::LiquidAsset).

In `@coincube-gui/src/app/state/spark/overview.rs`:
- Around line 73-83: The status computation currently prefers snapshot over
error, causing stale Connected(...) UI when get_info() fails; modify the logic
in the block that builds `status` so it checks `self.error` before
`self.snapshot` (i.e., if let Some(err) = &self.error =>
SparkStatus::Error(err.clone()) comes before the `if let Some(snapshot)` branch)
or ensure that wherever `self.error` is set (e.g., after `get_info()` fails) you
also clear `self.snapshot` so `SparkStatus::Connected(snapshot.clone())` cannot
be chosen when an error exists. Target the status construction logic around
`SparkStatus::Unavailable`, `SparkStatus::Loading`, `SparkStatus::Connected`,
and `SparkStatus::Error` to implement the change.

In `@coincube-gui/src/app/state/spark/receive.rs`:
- Around line 183-249: The generate completion handlers can overwrite a newer
form state because in-flight tasks from previous inputs still dispatch
GenerateSucceeded/GenerateFailed; add a monotonically-incremented request token
field on the SparkReceive state (e.g., generate_req_id: u64), increment it when
handling SparkReceiveMessage::GenerateRequested and capture the current token in
both Task::perform async closures for backend.receive_bolt11 and
backend.receive_onchain, then when mapping the result into
SparkReceiveMessage::GenerateSucceeded/GenerateFailed include the token and, in
the Update handling of those messages, only apply the result if the token
matches self.generate_req_id (otherwise ignore as stale). Update both the Bolt11
and Onchain branches to capture and propagate the token, and ensure other reset
paths (MethodSelected, AmountInputChanged, DescriptionInputChanged) also
increment or invalidate the token so late completions are discarded.

In `@coincube-gui/src/app/state/spark/send.rs`:
- Around line 115-179: The async prepare task can complete after the user edits
inputs and must be ignored if stale: add a monotonic prepare token (e.g.
prepare_version: u64) to the SparkSend state, increment it whenever
destination_input or amount_input changes (in the
SparkSendMessage::DestinationInputChanged and ::AmountInputChanged handlers and
when entering Preparing), capture the current token in the async Task that calls
resolve_and_prepare(…) and include that token in the returned message payload,
and when handling SparkSendMessage::PrepareSucceeded and ::PrepareFailed compare
the returned token against the current prepare_version before setting self.phase
(only apply results when they match); likewise ensure ConfirmRequested checks
the current prepared version if needed.

In `@coincube-gui/src/app/state/spark/settings.rs`:
- Around line 151-187: When dispatching
crate::app::view::SparkSettingsMessage::StableBalanceToggled, capture the
previous self.stable_balance_active (e.g. let prev = self.stable_balance_active)
and include it in the async Task result so StableBalanceSaved carries the
previous value along with the success/err; change the Task::perform closure to
return a tuple (Result<(), E>, Option<bool>) or otherwise encode the previous
state, and update the StableBalanceSaved match arm to on Err use that provided
previous value to restore stable_balance_active (instead of computing !current)
and set SparkSettingsStatus::Error with the error string. Ensure references to
set_stable_balance, StableBalanceToggled, StableBalanceSaved, and
stable_balance_active are updated accordingly.

In `@coincube-gui/src/app/view/spark/receive.rs`:
- Around line 268-274: The short_error function currently slices the string by
bytes using &err[..MAX], which can panic on multibyte UTF-8 characters; change
short_error to truncate on character boundaries by taking the first MAX chars
(e.g., using err.chars().take(MAX).collect::<String>() or using char_indices()
to find a safe byte index) and then append the ellipsis if truncated so the
function never slices mid-character.

In `@coincube-gui/src/gui/tab.rs`:
- Around line 341-351: The Loader::new calls in tab.rs are discarding an
existing Spark backend by passing None (e.g., the call that passes
i.breez_client.clone() followed by None); instead thread the current
spark_backend Option through these constructors the same way breez_client is
preserved: locate each Loader::new invocation (including the other occurrences
referenced) and replace the final None argument with the existing spark_backend
variable (cloned or referenced as appropriate), ensuring the Loader::new
signature is used consistently so an already-initialized Spark backend is not
overwritten.

---

Nitpick comments:
In `@coincube-gui/src/app/breez_spark/client.rs`:
- Around line 851-861: In the match arm handling
broadcast::error::RecvError::Closed inside the SparkClient subscription loop,
remove the unreachable break that follows std::future::pending::<()>().await
(the break after the pending await is dead code); leave the pending await to
park the task and optionally adjust the comment to explain the intent so the
code clearly reflects that the task is intentionally parked until the
subscription is dropped.
- Around line 668-674: The warn message and comment in the writer task (the
warn!("Spark bridge writer: stdin closed, draining remaining requests") and
surrounding comment) are misleading because the writer simply breaks and does
not drain the pending map; update either the comment/message to accurately say
the writer is stopping and the bridge is dead, or implement a shutdown signal so
the writer notifies the reader to exit early: add a shutdown channel (e.g.,
tokio::sync::watch or oneshot), have the writer send the shutdown when stdin is
closed and adjust the reader task to select on that shutdown signal and then
fail pending requests in the pending map immediately; ensure you update the warn
log text and reference the writer task, reader task, and pending map when making
changes.

In `@coincube-gui/src/app/state/liquid/send.rs`:
- Around line 789-833: Extract the USDt-detection logic into a shared helper
like fn usdt_amount_minor(details: &DomainPaymentDetails, usdt_id: &str) ->
Option<u64> (or similar) placed in a common module (e.g., wallets types or
breez_liquid assets), then replace the duplicated matches in
send.rs/overview.rs/receive.rs with calls to that helper; update send.rs to use
the helper to set is_usdt_payment, amount, and fiat_amount, and keep the
description branch using the helper result instead of re-matching
DomainPaymentDetails::LiquidAsset.

In `@coincube-ui/src/color.rs`:
- Line 54: WARN_ORANGE duplicates the same literal as DARK_ORANGE which can
drift; change WARN_ORANGE to alias DARK_ORANGE instead of repeating the hex
literal by assigning WARN_ORANGE = DARK_ORANGE (keep the same type Color and
const declaration), so any future changes to DARK_ORANGE automatically apply to
WARN_ORANGE; update the const named WARN_ORANGE to reference DARK_ORANGE rather
than using iced::color!(0xCA6719).
🪄 Autofix (Beta)

Fix all unresolved CodeRabbit comments on this PR:

  • Push a commit to this branch (recommended)
  • Create a new PR with the fixes

ℹ️ Review info
⚙️ Run configuration

Configuration used: defaults

Review profile: CHILL

Plan: Pro

Run ID: 1d834731-ce37-488c-87c4-110d8dfe5ff3

📥 Commits

Reviewing files that changed from the base of the PR and between 72ca626 and a6b27d0.

⛔ Files ignored due to path filters (1)
  • coincube-ui/static/logos/spark_badge.svg is excluded by !**/*.svg
📒 Files selected for processing (52)
  • .env.example
  • coincube-gui/src/app/breez_spark/client.rs
  • coincube-gui/src/app/breez_spark/config.rs
  • coincube-gui/src/app/breez_spark/mod.rs
  • coincube-gui/src/app/menu.rs
  • coincube-gui/src/app/mod.rs
  • coincube-gui/src/app/state/global_home.rs
  • coincube-gui/src/app/state/liquid/overview.rs
  • coincube-gui/src/app/state/liquid/receive.rs
  • coincube-gui/src/app/state/liquid/send.rs
  • coincube-gui/src/app/state/liquid/sideshift_receive.rs
  • coincube-gui/src/app/state/liquid/sideshift_send.rs
  • coincube-gui/src/app/state/mod.rs
  • coincube-gui/src/app/state/settings/lightning.rs
  • coincube-gui/src/app/state/settings/mod.rs
  • coincube-gui/src/app/state/spark/mod.rs
  • coincube-gui/src/app/state/spark/overview.rs
  • coincube-gui/src/app/state/spark/receive.rs
  • coincube-gui/src/app/state/spark/send.rs
  • coincube-gui/src/app/state/spark/settings.rs
  • coincube-gui/src/app/state/spark/transactions.rs
  • coincube-gui/src/app/view/global_home.rs
  • coincube-gui/src/app/view/liquid/overview.rs
  • coincube-gui/src/app/view/liquid/send.rs
  • coincube-gui/src/app/view/message.rs
  • coincube-gui/src/app/view/settings/general.rs
  • coincube-gui/src/app/view/settings/lightning.rs
  • coincube-gui/src/app/view/settings/mod.rs
  • coincube-gui/src/app/view/spark/mod.rs
  • coincube-gui/src/app/view/spark/overview.rs
  • coincube-gui/src/app/view/spark/receive.rs
  • coincube-gui/src/app/view/spark/send.rs
  • coincube-gui/src/app/view/spark/settings.rs
  • coincube-gui/src/app/view/spark/transactions.rs
  • coincube-gui/src/app/wallets/registry.rs
  • coincube-gui/src/app/wallets/spark.rs
  • coincube-gui/src/app/wallets/types.rs
  • coincube-gui/src/export.rs
  • coincube-gui/src/gui/tab.rs
  • coincube-gui/src/installer/mod.rs
  • coincube-gui/src/launcher.rs
  • coincube-gui/src/services/lnurl/stream.rs
  • coincube-spark-bridge/src/main.rs
  • coincube-spark-bridge/src/server.rs
  • coincube-spark-protocol/src/lib.rs
  • coincube-ui/src/color.rs
  • coincube-ui/src/component/button.rs
  • coincube-ui/src/image.rs
  • coincube-ui/src/theme/button.rs
  • coincube-ui/src/theme/palette.rs
  • docs/SPARK_WALLET.md
  • docs/WALLETS.md
✅ Files skipped from review due to trivial changes (3)
  • coincube-gui/src/app/state/spark/mod.rs
  • docs/WALLETS.md
  • coincube-gui/src/app/wallets/types.rs
🚧 Files skipped from review as they are similar to previous changes (15)
  • .env.example
  • coincube-gui/src/app/state/liquid/sideshift_receive.rs
  • coincube-gui/src/app/state/liquid/sideshift_send.rs
  • coincube-gui/src/app/view/liquid/overview.rs
  • coincube-gui/src/app/menu.rs
  • coincube-gui/src/app/view/spark/send.rs
  • coincube-gui/src/app/view/liquid/send.rs
  • coincube-gui/src/app/breez_spark/config.rs
  • coincube-gui/src/installer/mod.rs
  • coincube-gui/src/launcher.rs
  • coincube-gui/src/services/lnurl/stream.rs
  • coincube-gui/src/app/view/spark/transactions.rs
  • coincube-gui/src/app/view/spark/overview.rs
  • coincube-gui/src/app/wallets/spark.rs
  • coincube-spark-protocol/src/lib.rs

Comment thread coincube-gui/src/app/breez_spark/mod.rs
Comment thread coincube-gui/src/app/breez_spark/mod.rs
Comment on lines +1134 to +1142
Err(e) => {
tracing::warn!("Home: spark get_info failed: {}", e);
// Soft-fail: leave the card showing whatever the
// last successful fetch returned. No hard error
// because the Spark card is non-essential to the
// rest of the home page.
Message::View(view::Message::Home(HomeMessage::SparkBalanceUpdated(
Amount::ZERO,
)))
Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

⚠️ Potential issue | 🟠 Major

Don't zero the Spark balance on a transient get_info failure.

Lines 1136-1139 say the home page should keep the last successful value, but the error branch immediately emits SparkBalanceUpdated(Amount::ZERO). A single bridge hiccup will therefore flash the wallet to 0 sats and can make the new Spark card look empty. Keep the cached balance unchanged on failure, or send a separate no-op/error message instead.

🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.

In `@coincube-gui/src/app/state/global_home.rs` around lines 1134 - 1142, The
Err(e) branch currently emits
Message::View(view::Message::Home(HomeMessage::SparkBalanceUpdated(Amount::ZERO)))
which incorrectly zeros the cached Spark balance on transient get_info failures;
change the Err(e) handling in that match to avoid dispatching
SparkBalanceUpdated(Amount::ZERO) — either do not emit any balance-update
message (so the cached balance remains unchanged) or emit a dedicated
error/no-op message (e.g., HomeMessage::SparkBalanceFetchFailed or a no-op
HomeMessage variant) instead of using Amount::ZERO; update the Err(e) arm that
contains tracing::warn! to only log the error and return the non-updating/no-op
message.

Comment on lines +77 to +99
update_settings_file(&network_dir, |mut settings| {
if let Some(entry) = settings.cubes.iter_mut().find(|c| c.id == cube_id) {
entry.default_lightning_backend = kind;
} else {
tracing::error!(
"Cube not found (id={}) — cannot save default_lightning_backend",
cube_id
);
}
Some(settings)
})
.await
.map_err(|e| e.to_string())
},
|result| match result {
Ok(()) => Message::SettingsSaved,
Err(err) => {
tracing::warn!("Failed to persist default_lightning_backend: {}", err);
Message::View(view::Message::ShowError(format!(
"Failed to save Lightning backend: {}",
err
)))
}
Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

⚠️ Potential issue | 🟠 Major

Don't emit SettingsSaved when the cube lookup misses.

Lines 78-86 only log the missing cube_id, but Lines 91-93 still convert the write into Message::SettingsSaved. That makes the UI report a successful save even though default_lightning_backend was never updated. Return an error for the missing-cube case so the failure path is surfaced instead.

🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.

In `@coincube-gui/src/app/state/settings/lightning.rs` around lines 77 - 99, The
code always returns Some(settings) from the update_settings_file closure even
when the cube lookup fails, so the outer result becomes Ok(()) and the UI emits
Message::SettingsSaved; change the closure so that when
settings.cubes.iter_mut().find(|c| c.id == cube_id) is None you do not return
Some(settings) but instead return None (or otherwise propagate an error) so
update_settings_file will fail and the match will take the Err branch (showing
the failure message) rather than emitting Message::SettingsSaved; target the
closure passed to update_settings_file around the cube lookup and
default_lightning_backend assignment and replace the logging-only path with a
non-Some return.

Comment on lines +119 to +123
(Ok(info), Ok(list)) => Message::View(view::Message::SparkOverview(
view::SparkOverviewMessage::DataLoaded {
balance: Amount::from_sat(info.balance_sats),
recent_payments: list.payments,
},
Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

⚠️ Potential issue | 🟠 Major

Don't drop identity_pubkey during reload.

get_info() is fetched here, but DataLoaded only carries balance, so Line 164 can only preserve the previous identity_pubkey. On first load this leaves snapshot.identity_pubkey empty forever. Include the pubkey in the loaded payload, or remove it from SparkBalanceSnapshot if the view does not need it.

Also applies to: 164-171

🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.

In `@coincube-gui/src/app/state/spark/overview.rs` around lines 119 - 123, The
reload flow drops identity_pubkey because get_info() is used but
view::SparkOverviewMessage::DataLoaded only carries balance and recent_payments;
update the DataLoaded payload (and its construction in the match arm where
get_info() and list are Ok) to include identity_pubkey from info (e.g., pass
info.identity_pubkey into DataLoaded) and propagate it through
SparkBalanceSnapshot, or if the view truly does not need identity_pubkey, remove
identity_pubkey from the SparkBalanceSnapshot and its usages; change the
DataLoaded variant and any constructors/consumers (e.g., where DataLoaded is
matched/handled and SparkBalanceSnapshot is created) accordingly so the pubkey
is not lost on first load.

Comment on lines +255 to +258
self.qr_data = qr_code::Data::new(&ok.payment_request).ok();
// Phase 4f: capture the BOLT11 string so PaymentReceived
// events can correlate against THIS invoice.
self.displayed_invoice = Some(ok.payment_request.clone());
Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

⚠️ Potential issue | 🟠 Major

On-chain receives never satisfy the payment correlation check.

GenerateSucceeded stores displayed_invoice for every method, but PaymentReceived only accepts bolt11 == None when displayed_invoice is None. In the on-chain flow, displayed_invoice becomes the deposit address, so the later event is always rejected and the panel never reaches Received. Only populate displayed_invoice for BOLT11, or add an explicit on-chain correlation path.

Also applies to: 285-289

🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.

In `@coincube-gui/src/app/state/spark/receive.rs` around lines 255 - 258,
GenerateSucceeded currently sets displayed_invoice for all receive methods
causing PaymentReceived to reject on-chain receipts because PaymentReceived only
treats bolt11 == None as valid when displayed_invoice is None; change
GenerateSucceeded (the branch that sets self.displayed_invoice) to only populate
self.displayed_invoice when the payload is a BOLT11 invoice (i.e., when
ok.payment_request is a bolt11 string), leaving displayed_invoice as None for
on-chain/deposit-address flows, or alternatively add an explicit correlation in
PaymentReceived to accept matches where the event carries an on-chain deposit
address; update both spots mentioned (the block that sets self.displayed_invoice
around lines shown and the similar block at 285-289) to implement the
BOLT11-only assignment and ensure PaymentReceived's matching logic still uses
displayed_invoice and bolt11 consistently.

Comment on lines +83 to +85
let stable_balance_card =
stable_balance_card(self.stable_balance_active, self.stable_balance_saving, true);
let bridge_status_card = bridge_status_card(&self.status);
Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

⚠️ Potential issue | 🟠 Major

Disable the Stable Balance controls when the bridge is not reachable.

stable_balance_card is always called with true, so SparkSettingsStatus::Error(_) still leaves the toggle enabled once stable_balance_active is known. That lets the user keep firing set_stable_balance RPCs against a bridge we already marked unreachable.

Suggested fix
-        let stable_balance_card =
-            stable_balance_card(self.stable_balance_active, self.stable_balance_saving, true);
+        let spark_available = matches!(self.status, SparkSettingsStatus::Connected);
+        let stable_balance_card = stable_balance_card(
+            self.stable_balance_active,
+            self.stable_balance_saving,
+            spark_available,
+        );
📝 Committable suggestion

‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.

Suggested change
let stable_balance_card =
stable_balance_card(self.stable_balance_active, self.stable_balance_saving, true);
let bridge_status_card = bridge_status_card(&self.status);
let spark_available = matches!(self.status, SparkSettingsStatus::Connected);
let stable_balance_card = stable_balance_card(
self.stable_balance_active,
self.stable_balance_saving,
spark_available,
);
let bridge_status_card = bridge_status_card(&self.status);
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.

In `@coincube-gui/src/app/view/spark/settings.rs` around lines 83 - 85,
stable_balance_card is always invoked with a hardcoded `true`, so the
stable-balance toggle remains enabled even when the bridge is unreachable
(SparkSettingsStatus::Error(_)); change the call to compute the enabled flag
from the bridge status (e.g. match on `self.status` or add an `is_reachable()`
helper) and pass that boolean instead of `true`, ensuring the toggle (and thus
calls to set_stable_balance) are disabled when `self.status` indicates an
error/unreachable bridge; update the invocation of
`stable_balance_card(self.stable_balance_active, self.stable_balance_saving,
true)` to use the computed reachable boolean derived from `self.status`.

Comment on lines +33 to +35
/// Required env vars: `BREEZ_API_KEY`, `COINCUBE_SPARK_MNEMONIC`,
/// `COINCUBE_SPARK_STORAGE_DIR`.
#[arg(long)]
Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

⚠️ Potential issue | 🟡 Minor

Make --smoke-test honor the Spark API key fallback too.

This path still hard-requires BREEZ_API_KEY, but the PR introduces BREEZ_SPARK_API_KEY with fallback to BREEZ_API_KEY. If someone follows the new env example, the smoke test will fail even though the rest of the Spark flow is configured correctly. Prefer BREEZ_SPARK_API_KEY first and fall back to BREEZ_API_KEY.

Also applies to: 69-76

🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.

In `@coincube-spark-bridge/src/main.rs` around lines 33 - 35, The smoke-test path
still requires BREEZ_API_KEY directly instead of using the new
BREEZ_SPARK_API_KEY fallback; update the code that reads the Spark API key (the
places that currently read BREEZ_API_KEY for --smoke-test, referenced around the
arg declaration and the smoke-test handling near lines 69-76) to first try env
var BREEZ_SPARK_API_KEY and if missing fall back to BREEZ_API_KEY, and use that
resulting value wherever the smoke-test checks or passes the Spark API key
(e.g., the variables/params named like spark_api_key or breez_api_key used in
the smoke-test logic).

Comment thread docs/SPARK_WALLET.md
Comment on lines +13 to +25
```
┌──────────────┐ stdin/stdout ┌──────────────────────┐
│ coincube-gui │ ◄──────── JSON-RPC ──────────► │ coincube-spark-bridge │
│ (iced UI) │ │ (breez-sdk-spark) │
└──────────────┘ └──────────────────────┘
│ │
│ links: breez-sdk-liquid │ links: spark-sdk
│ │
▼ ▼
┌──────────────┐ ┌──────────────────┐
│ Liquid SDK │ │ Spark SDK │
└──────────────┘ └──────────────────┘
```
Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

⚠️ Potential issue | 🟡 Minor

Add a language hint to the fenced diagram block.

This fence is missing a language, so markdownlint MD040 will keep firing on the docs change. text would be enough here.

🧰 Tools
🪛 markdownlint-cli2 (0.22.0)

[warning] 13-13: Fenced code blocks should have a language specified

(MD040, fenced-code-language)

🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.

In `@docs/SPARK_WALLET.md` around lines 13 - 25, The fenced ASCII diagram block in
docs/SPARK_WALLET.md is missing a language hint which triggers markdownlint
MD040; update the opening fence for the diagram (the triple-backtick block
around the coincube-gui / coincube-spark-bridge diagram) to include a language
hint such as "text" (i.e., change ``` to ```text) so the linter recognizes it as
a text/code block.

Comment thread docs/SPARK_WALLET.md
Comment on lines +37 to +38
- The gui spawns the bridge lazily when the cube has a `spark_wallet_signer_fingerprint` set. If the spawn fails (binary missing, handshake error) the `WalletRegistry` holds `spark = None`, and all Spark panels render an "unavailable" stub.
- The bridge binary is located via the `COINCUBE_SPARK_BRIDGE_PATH` env var, or falls back to `coincube-spark-bridge` sitting alongside the main `coincube` binary in the same directory.
Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

⚠️ Potential issue | 🟡 Minor

The Spark wallet docs no longer match the implemented settings/UI.

The code now loads both SDKs from cube.breez_wallet_signer_fingerprint, and the default-backend picker moved to Settings → Lightning. These sections still point readers at spark_wallet_signer_fingerprint / liquid_wallet_signer_fingerprint and Spark → Settings, so the setup and troubleshooting steps send users to the wrong knobs.

Also applies to: 49-52, 74-75, 88-90

🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.

In `@docs/SPARK_WALLET.md` around lines 37 - 38, Update the Spark wallet docs to
match the current code and UI: replace references to
spark_wallet_signer_fingerprint and liquid_wallet_signer_fingerprint with
cube.breez_wallet_signer_fingerprint, change any "Spark → Settings" instructions
to "Settings → Lightning" (or the new default-backend picker location), and
adjust setup/troubleshooting text that describes how the GUI spawns the bridge
to mention the code now loads both SDKs from
cube.breez_wallet_signer_fingerprint; ensure all occurrences (including the ones
noted around lines 49-52, 74-75, 88-90) are updated and that guidance about
COINCUBE_SPARK_BRIDGE_PATH and the bridge spawn behavior remains accurate with
the new loading logic.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant